[4/15] Fix result modes in ifelse(), t(), and diag(); enforce ifelse() branch shapes - #140
Conversation
bd43518 to
d9bbeee
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 93.17% 93.26% +0.08%
==========================================
Files 28 29 +1
Lines 6083 6161 +78
==========================================
+ Hits 5668 5746 +78
Misses 415 415 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fortran's merge() requires same-typed branches, but the ifelse() handler took its result mode from `yes` alone and never cast `no`: mixed-mode branches emitted an invalid mixed-type merge() (ifelse(c, 1L, a) with double `a` failed at the gfortran stage). The result shape also came from the first non-scalar of (test, yes, no), where R documents ifelse() as returning a result shaped like `test`. Promote both branches to their common lattice mode with promote_operands(), and take the result's mode and shape from the promoted branches and the (booleanized) test respectively. A scalar test with array-valued branches is now a compile-time error -- merge() cannot represent R's length-1 result -- instead of silently emitting branch-shaped code.
R's t() and diag() preserve their input's type, but the handlers unconditionally cast to double: t(m) and diag(m) on an integer matrix returned doubles, and the constructor forms diag(x) / diag(x, nrow, ncol) lost typeof(x) too. Only the identity forms (diag(n), diag(nrow = n)) are double in R, which is what quickr already emits for them. Drop the maybe_cast_double() calls and carry the input mode through the result Variable, the hoisted temporaries, and the zero fill in diag_matrix(). can_use_output() gains a `mode` argument (default "double"; all other callers unchanged) so an in-place destination is only used when its declared mode matches, and infer_dest_diag() reports the input's mode instead of hard-coding double -- a double-inferred dest would have mislabeled an integer result's declaration. The transpose casts in unwrap_transpose_arg() stay, now with a comment saying why: that path only feeds matrix products, which always return double in R.
ifelse() promoted branch modes and rejected scalar-test/array-branch calls, but never validated branch shapes against `test`. Fortran's merge() requires conformable arguments, so a runtime length mismatch read past the shorter branch and returned garbage where R recycles. A non-scalar branch must now match the shape of `test`: statically unequal dims (including rank mismatches) are a compile error, and symbolic dims emit a statement-level runtime size guard, matching the elementwise-operator policy. NA dims always count as unknown -- two unknown lengths are not the same quantity.
d9bbeee to
e1b3e77
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1b3e77ce2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Coverage flagged both stop() calls in check_ifelse_branch_shape() as untested. The rank check is reachable: a matrix branch under a vector `test`, or the reverse, hits it, and the existing mismatch test only exercises the per-axis verdict. Add a case for each direction. The `is.null(hoist)` arm is not reachable. r2f() replaces a NULL hoist with a fresh one before dispatching to any handler, and the only other dispatch route resolves `f<-`-style names, so the ifelse() handler always has a hoist to emit the size guard into. Delete it and record the invariant in the function's comment rather than testing dead code.
|
Fixed code coverage and removed an unreachable branch. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab11aa00b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fixes #125.
What changed
Three handlers now match R's result types (and, for
ifelse(), R'sresult shape):
ifelse()promotes its branches and shapes the result liketest.Mixed-mode branches previously emitted an invalid mixed-type
merge()(Fortran requires same-typed branches), so this failed at the gfortran
stage:
The result's shape now comes from
test, per R's documented contract,instead of from the first non-scalar of
(test, yes, no). One casebecomes a compile-time error: a scalar
testwith array-valuedbranches (R returns a length-1 result there, which
mergecannotexpress — previously this silently emitted a branch-shaped result):
ifelse()enforces branch shapes. Fortran'smerge()requiresconformable arguments, so a runtime length mismatch between
testand anon-scalar branch read past the shorter branch and returned garbage
(
c(10, 2, 4.65e-310)where R recycles). A non-scalar branch must nowmatch the shape of
test: statically unequal dims (including rankmismatches) are a compile error, and symbolic dims emit a statement-level
runtime size guard — the same three-valued policy the elementwise
operators adopt later in this series.
NAdims always count as unknown(two unknown lengths are not the same quantity). Scalar branches
broadcast natively, as before. R-style branch recycling is deliberately
not implemented — refusal keeps the "matches R or errors" invariant.
t()anddiag()preserve the input mode. Both unconditionallycast to double; R preserves the input's type:
This covers
t(<matrix>),t(<vector>),diag(<matrix>)(diagonalextraction), and the constructor forms
diag(x)/diag(x, nrow, ncol)(R preserves
typeof(x):diag(1:3)is integer,diag(TRUE, 2)islogical). The identity forms (
diag(n),diag(nrow = n)) stay double,matching R.
Not changed, deliberately: the transposes feeding matrix products
(
%*%,crossprod, ...) keep their double casts — R's matrix productsalways return double — with a comment added saying why.
How
R/r2f-conditionals.R: theifelsehandler promotes both branchesthrough
promote_operands()(the helper introduced for operators andc()), takes dims from the booleanized mask, and errors on thescalar-test/array-branch case. Branch shapes are checked per axis
(
ifelse_axis_verdict()); unknown axes are guarded throughemit_quickr_error_if(), combined into one.or.condition perbranch.
R/r2f-matrix.R,R/r2f-matrix-blas.R:t(),diag_extract(), anddiag_matrix()drop theirmaybe_cast_double()and carry the inputmode through the result
Variable, hoisted temporaries, anddiag_matrix()'s zero fill.R/r2f-matrix-blas.R:can_use_output()gains amodeargument(default
"double", all other callers unchanged) so an in-placedestination is only used when its declared mode matches the result.
R/r2f-matrix-infer.R:infer_dest_diag()reports the input's modeinstead of hard-coding double, so
out <- diag(m)declaresoutwith the right mode on the inferred-destination path too. When the
input's mode is not yet known it reports no destination rather than
guessing one.
diag_matrix()'s zero fill is spelled per mode (0_c_int,.false.,…) and refuses modes it cannot spell.
Tests
test-ifelse.R: mixed-mode branches compile and match R (includingtypeof()), logical branches join as logical, and thescalar-test/array-branch case raises the new error. A translation
snapshot locks the promoted
mergespelling. New shape tests: astatically mismatched branch is a compile error, unknown-length
branches guard at runtime (conformable inputs still match R;
mismatched inputs raise instead of returning garbage), with a
snapshot pinning the guard text.
test-matrix.R:t()anddiag()preserve integer and logicalmodes across the matrix, vector, rectangular-with-recycling, and
inferred-destination (
out <- diag(m)) paths; the identity formstays double. A snapshot locks the integer
diagextraction writingdirectly into the integer output.
No existing snapshots changed; the only snapshot additions are the two
new tests above.
Notes for review
ifelse()with scalartestand non-scalarbranches now errors at compile time instead of returning a
branch-shaped result (a silent divergence from R before).
ifelse()with branch lengths that differ fromtestnow errors (compile-time when static, runtime when symbolic)instead of recycling in R and reading out of bounds in quickr.
typeof()changes (double → integer/logical) fort()anddiag()on non-double inputs — previously a silent divergence from R.diag()'s constructor forms preservetypeof(x)in (at leastreasonably modern) R —
typeof(diag(1:3))is"integer"— so thoseare fixed here too, not just
diag(<matrix>).